Review: fix(router): avoid repeated App path decoding

I traced the full request path for App Router across dev (index.ts), Node prod (prod-server.ts), and the generated RSC entry (which the Workers path also delegates to). This is a correct and well-targeted fix for a real double-decode / middleware-bypass class of bug. Verdict: approve with a couple of non-blocking observations.

What the fix does (and why it's sound)

The change has two complementary parts:

1. Idempotent segment decode (routing/utils.ts) — broadening PATH_DELIMITER_REGEX from %(2f|23|3f|5c) to %[0-9a-f]{2} makes decodeRouteSegment/decodeRouteSegmentStrict re-encode every residual %XX escape after decodeURIComponent, not just delimiters. I verified this makes the function fully idempotent across a wide input set (normal UTF-8, %2F, double-encoded %2561, traversal shapes ..%252fadmin, malformed %C3/%2G, bare %): decode(x) === decode(decode(x)) for all of them. This is the crux — even code paths that still decode twice (e.g. findIntercept, the un-migrated matchRoute calls in app-server-action-execution.ts) now produce stable results.

2. matchNormalizedRoute fast path (app-rsc-route-matching.ts + handler) — when cleanPathname is still the request-boundary-normalized value, route matching skips the redundant re-decode via splitPathSegments. The cleanPathnameIsNormalized flag is correctly flipped to false on every path that mutates cleanPathname to an un-normalized value: middleware rewrite (gated on middlewareResult.rewritten, which — verified in app-middleware.ts — is the only condition under which cleanPathname changes), and each beforeFiles/afterFiles/fallback/action rewrite. Because the boundary value is guaranteed decoded-once (via normalizePathnameForRouteMatchStrict in normalizeRscRequest), and decode is now idempotent, matchNormalizedRoute (skip decode) and matchRoute (decode once) are provably equivalent on normalized input.

Dev/prod/worker parity — verified

• prod-server: reverting the RSC handler call from normalizedUrl back to rawUrl is the right move. Static-asset lookup and the /__vinext/ prerender gate still use the decoded pathname, but the RSC handler now decodes exactly once inside normalizeRscRequest instead of receiving an already-decoded path and decoding again. The prerender-secret gate stays consistent because %2F survives decoding in both prod-server and the handler.
• dev (index.ts): passing pathname (decoded once at line 4731) as the new normalizedPathname arg prevents executeMiddleware → resolveMiddlewarePathname from decoding request.url a second time. This is the actual dev-side bug being closed — previously /%2561dmin could decode twice to /admin and hit a middleware matcher it should not.
• workers: inherits the fix via the built RSC entry; covered by the new cloudflare-workers/ssr.spec.ts cases.

Security scenario is legit

The /%2561dmin case is a genuine middleware-bypass fix: Next.js matches middleware against the decode-once pathname (/%61dmin != /admin, so middleware does not block), and route matching must not decode again to /admin. The traversal cases (..%252fadmin, /api/health/..%252fadmin) correctly stay 404 because %252f remains encoded through decode+normalizePath, so .. can't traverse to /admin. I confirmed all three assertions by simulation.

I ran tests/app-rsc-route-matching.test.ts (27 pass), app-router-malformed-url + app-router-middleware-next-request (20 pass), entry-templates (41 pass), and tsc --noEmit (clean).

Non-blocking observations

1. app-server-action-execution.ts still calls options.matchRoute(options.cleanPathname) (lines 1284, 1420) with an already-normalized cleanPathname, i.e. it double-decodes. This is now harmless thanks to the idempotency fix, but it's an inconsistency with the handler's matchCleanPathname optimization. Not a regression (pre-existing), and out of scope — but worth a follow-up so the "match a normalized path" contract is applied uniformly. A code comment noting these intentionally rely on decode idempotency would help future readers.

2. The matchNormalizedRoute perf win is entirely dependent on decode idempotency for correctness. Since the idempotency fix alone would restore correctness, the fast path is a pure optimization. Consider a one-line comment at the matchCleanPathname definition stating that skipping decode is safe because the boundary value is already strict-normalized (so callers don't accidentally route an encoded path through it after a future refactor).

3. api/health route doesn't exist in the fixture, so /api/health/..%252fadmin is really just exercising "unknown route → 404". That's fine for the traversal assertion, but if the intent was to prove traversal can't reach an existing sibling route, pointing the second traversal case at a real route (or the existing /api/hello) would make the test stronger.

Overall: clean, minimal, correct, and the idempotency insight makes the whole thing robust rather than relying on catching every call site. Nice work.

Review: fix(router): avoid repeated App path decoding

I independently re-verified this fix from the ground up: the decode regex change, the fast-path flag lifecycle, dev/prod/worker parity, and the security scenarios. Verdict: approve. The fix is correct, minimal, and closes a real double-decode / middleware-bypass bug class. A couple of non-blocking robustness notes below.

The core mechanism is sound

1. Idempotent segment decode (routing/utils.ts:172). Broadening PATH_DELIMITER_REGEX from %(2f|23|3f|5c) to %[0-9a-f]{2} re-encodes every residual %XX escape after decodeURIComponent, not just delimiters. I confirmed decode(x) === decode(decode(x)) across normal UTF-8 (caf%C3%A9, %E2%9C%93), delimiters (%2F), double-encoded (%2561), percent literals (100%25), and plus/space. The net effect on a double-encoded segment is identity: %2561dmin → (decode) %61dmin → (re-encode) %2561dmin.

2. Full-pipeline correctness (this is the actual bug being fixed). The decode-once-at-boundary + decode-again-at-param pipeline is what produced the bypass. I simulated the end-to-end path:

The OLD pipeline turned /%2561dmin into admin — a genuine middleware bypass. NEW keeps single-encoded paths decoding correctly while stopping double-encoded paths from collapsing to the target. This matches Next.js semantics: native URL.pathname does not decode, so Next.js middleware sees the raw double-encoded path; vinext decodes exactly once for matching and preserves residuals.

Parity verified across all three runtimes

• prod-server (prod-server.ts:1432): reverting the RSC handler URL from normalizedUrl back to rawUrl is correct — static-asset lookup still uses decoded pathname, but the RSC handler now decodes exactly once inside normalizeRscRequest instead of receiving a pre-decoded path and decoding again.
• dev (index.ts:4962): passing the once-decoded pathname as normalizedPathname to the Pages runner's runMiddleware prevents resolveMiddlewarePathname from decoding request.url a second time. This is the dev-side bug. App Router middleware was already safe (it passes normalizedPathname: cleanPathname in app-middleware.ts:249).
• workers: inherits via the built RSC entry; covered by the new cloudflare-workers/ssr.spec.ts cases.

Fast-path flag lifecycle is complete

matchNormalizedRoute (skips the redundant decodeRouteSegment via splitPathSegments) is a pure optimization gated by cleanPathnameIsNormalized. I checked all six cleanPathname = assignments in app-rsc-handler.ts (642, 685, 721, 746, 963, 1009) — every un-normalizing mutation clears the flag. The middleware case (642) gates the reset on middlewareResult.rewritten, and I verified in app-middleware.ts:214-232 that cleanPathname is only reassigned when rewritten = true, so the invariant holds. Since the boundary value is strict-normalized and decode is idempotent, matchNormalizedRoute (decode-once) and matchRoute (decode-twice) are provably equivalent on normalized input — I confirmed identical param results for a%61, caf%C3%A9, a%2561, 50%2525, %2F, a%2Fb.

Verification run

• tests/app-rsc-route-matching.test.ts + tests/shims.test.ts: 1212 pass
• tsc --noEmit on packages/vinext: clean

Non-blocking notes

1. Fragile cross-module invariant at app-rsc-handler.ts:642-643. Line 642 unconditionally assigns cleanPathname = middlewareResult.cleanPathname, but the flag reset on 643 is gated on rewritten. This is correct only because applyAppMiddleware maintains "cleanPathname changes ⟺ rewritten." If a future refactor normalized/mutated cleanPathname without setting rewritten, the fast path would silently route an un-normalized path through matchNormalizedRoute. Consider a value-based guard (if (cleanPathname !== normalized.cleanPathname) cleanPathnameIsNormalized = false) or a comment documenting the dependency.

2. app-server-action-execution.ts still double-decodes via options.matchRoute(options.cleanPathname) on an already-normalized path. Now harmless thanks to idempotency, but inconsistent with the handler's matchCleanPathname optimization. Worth a follow-up + a comment noting these intentionally rely on decode idempotency.

3. Traversal e2e tests only prove "unknown → 404." /foo/..%252fadmin and /api/health/..%252fadmin don't map to real traversal sources, so they exercise "unknown route → 404" rather than "traversal can't reach an existing sibling." The not.toContain("Protected admin content") assertion is the real security check and it holds; pointing one case at an existing route would make the intent sharper.

Clean, minimal, correct — the idempotency insight makes the whole thing robust rather than relying on catching every call site. Nice work.

Review: fix(router): avoid repeated App path decoding

I reviewed the current state of the branch (the implementation has evolved considerably from the earlier PATH_DELIMITER_REGEX-broadening approach described in prior comments — it now uses a dedicated raw-matching path). I traced the full request pipeline across dev (index.ts), Node prod (prod-server.ts), the generated RSC entry (Workers inherits it), and the route matcher. Verdict: approve. The fix is correct and well-scoped. A few non-blocking notes below.

What the fix actually does now

Rather than making decodeRouteSegment idempotent, this version introduces a parallel raw matching path:

1. matchRequestRoute + trieMatchRaw/matchRoutePatternRaw (app-rsc-route-matching.ts, route-trie.ts, route-pattern.ts) — route matching runs against the encoded requestCleanPathname split with splitPathSegments (no decode). Params are then finalized with a match-once contract: routeHandler routes get decodeMatchedParams (decode once), page routes get canonicalizeAppPageParams (decode→re-encode, i.e. kept encoded in canonical form). This matches Next.js, where route handlers receive decoded params but page params retain encoding.

2. requestCleanPathname (app-rsc-request-normalization.ts) — the original encoded pathname with only basePath + .rsc stripped, threaded through so the RSC handler can route on the un-decoded path. matchCleanPathname() uses it only while cleanPathnameIsRequestPathname holds; the flag is cleared on every cleanPathname mutation (middleware rewrite gated on rewritten, and each beforeFiles/afterFiles/fallback/action rewrite). I checked all six assignment sites — coverage is complete.

3. Middleware sees the encoded path (middleware-runtime.ts) — createNextRequest now builds NextRequest from raw url.pathname instead of normalizedPathname, while the matcher (matchPathname) still uses the decoded normalizedPathname. That is exactly Next.js semantics: the matcher decides whether middleware runs (decoded), but request.nextUrl.pathname reflects the encoded request. The new !hasBasePath(requestPathname, basePath) guard also correctly avoids double-prefixing basePath.

Parity — verified

• prod-server: reverting the RSC-handler URL from normalizedUrl back to rawUrl is correct — static-asset lookup still uses decoded pathname, but the handler now decodes exactly once inside normalizeRscRequest.
• dev (index.ts): the new configureServer middleware captures req.__vinextOriginalEncodedUrl before Vite normalizes, and middlewareUrl is threaded to runMiddleware(..., pathname). This closes the dev-side re-decode.
• workers: inherits via the built RSC entry; covered by the new cloudflare-workers/ssr.spec.ts cases.

Security scenarios check out

The interesting interaction: /%2561dmin — the matcher decodes to /admin so the /admin middleware matcher fires, but the middleware body reads request.nextUrl.pathname === "/%61dmin" (encoded), so pathname === "/admin" is false → no block → falls through to matchRequestRoute("/%2561dmin") → 404. Internally consistent, and matches Next.js. I also confirmed the raw path is stricter on traversal: requestCleanPathname is not normalizePath-collapsed, so .. stays a literal segment and cannot resolve to a sibling route (e.g. /foo/..%252fadmin → segments ["foo","..%252fadmin"] → 404). //-collapsing via splitPathSegments is harmless because the protocol-relative guard runs first and the normalized cleanPathname collapses identically.

Verification run (locally)

• tests/app-rsc-route-matching.test.ts (27), app-rsc-handler.test.ts (123), app-page-request.test.ts (29), prerender-route-params.test.ts (24) — all pass
• tests/shims.test.ts (1186) + static-image-emission.test.ts (5) — all pass
• tests/middleware-runtime.test.ts + app-router-middleware-next-request.test.ts + middleware-runtime-trailing-slash.test.ts (45) — all pass
• vp check on all changed source files — format/lint/types clean

Non-blocking notes

1. Dead parameter: createNextRequest's normalizedPathname (middleware-runtime.ts:227). Since mwPathname now derives from url.pathname, the normalizedPathname argument is no longer read inside createNextRequest (only referenced in a stale comment at :238). It still passes lint because of the comment/call-site, but it is genuinely unused now. Consider dropping the parameter (and updating the comment) so the function signature doesn't imply it participates in mwPathname construction.

2. middlewareUrl may retain a stripped .html suffix in dev (index.ts:4718). middlewareUrl is seeded from req.__vinextOriginalEncodedUrl (captured raw in configureServer), so it is not subject to the /index.html and .html stripping applied to url just above (lines 4711–4715). The _next/data branch explicitly resyncs middlewareUrl = url, but the .html case doesn't, so a /about.html request would hand middleware a nextUrl.pathname ending in .html. This is an unlikely path for App Router middleware, but it is an asymmetry with the non-encoded url and worth a resync (or a comment explaining why it's intentional).

3. Traversal e2e cases still target non-existent routes. /foo/..%252fadmin and /api/health/..%252fadmin don't map to real traversal sources, so they mostly assert "unknown route → 404". The not.toContain("Protected admin content") check is the real security assertion and it holds; pointing one case at an existing sibling would sharpen intent.

Overall: correct, minimal, good test coverage across all three runtimes, and the raw-match-with-canonicalization approach cleanly separates "decode for matcher gating" from "route on encoded path." Nice work.

Review: fix(router): avoid repeated App path decoding

I reviewed the current branch state end-to-end: the raw-matching path (matchRequestRoute / trieMatchRaw / matchRoutePatternRaw), the requestCleanPathname threading, the cleanPathnameIsRequestPathname flag lifecycle, createNextRequest's encoded-pathname switch, dev/prod/worker parity, and the security scenarios. I ran the affected unit suites and tsc/lint locally. Verdict: approve — correct, well-scoped, and thoroughly tested. Two non-blocking cleanliness notes below.

What the fix does (verified sound)

1. Dedicated raw-matching path (app-rsc-route-matching.ts, route-trie.ts, route-pattern.ts) — matchRequestRoute splits requestCleanPathname with splitPathSegments (no decode), then finalizes params with a match-once contract: route handlers get decodeMatchedParams (decode once → /api/a%2Fb yields a/b), page routes get canonicalizeAppPageParams (decode→re-encode → /files/a%2561 stays a%2561, %2F stays encoded). This matches Next.js, where route handlers receive decoded params but page params retain encoding. I confirmed all cases against the new unit tests.

2. requestCleanPathname (app-rsc-request-normalization.ts) — the original encoded pathname with only basePath + .rsc stripped, threaded so the RSC handler can route on the un-decoded path. matchCleanPathname() uses it only while cleanPathnameIsRequestPathname holds. I checked all six cleanPathname mutation sites in app-rsc-handler.ts (648, 690, 727, 752, 972, 1018) — every un-normalizing mutation clears the flag (middleware gated on rewritten, each beforeFiles/afterFiles/fallback rewrite unconditionally). Coverage is complete.

3. Middleware sees the encoded path (middleware-runtime.ts) — createNextRequest now builds mwPathname from url.pathname (raw encoded) instead of normalizedPathname, while the matcher (matchPathname in executeMiddleware) still gates on the decoded normalizedPathname. This is exactly Next.js semantics: the matcher decides whether middleware runs (decoded), but request.nextUrl.pathname reflects the encoded request. The new !hasBasePath(requestPathname, basePath) guard correctly avoids double-prefixing basePath for App Router (raw URL still carries basePath) while re-adding it for Pages dev (Vite-stripped URL). Verified both call paths.

Parity — verified across all three runtimes

• prod-server (prod-server.ts:1432): reverting the RSC-handler URL from normalizedUrl back to rawUrl is correct — static-asset lookup still uses the decoded pathname, but the handler now decodes exactly once inside normalizeRscRequest.
• dev (index.ts): configureServer captures req.__vinextOriginalEncodedUrl before Vite normalizes; middlewareUrl is threaded to runMiddleware(..., pathname), closing the dev-side re-decode. The basePath resync at 4768-4774 keeps middlewareUrl stripped in parity with url.
• workers: inherits via the built RSC entry; covered by the new cloudflare-workers/ssr.spec.ts cases.

Security scenarios check out

• /%2561dmin (double-encoded) → matcher decodes to /admin so the /admin middleware matcher fires, but the middleware body reads request.nextUrl.pathname === "/%61dmin" (encoded), so === "/admin" is false → no block → matchRequestRoute("/%2561dmin") → 404. Internally consistent, matches Next.js.
• /%61dmin (single-encoded) → same gating; body sees /%61dmin, raw matcher doesn't decode → 404. The getRawPath helper is a nice touch since Playwright's request normalizes the URL.
• Traversal (..%252fadmin): requestCleanPathname is not normalizePath-collapsed, so .. stays a literal segment and matchRequestRoute can't traverse to a sibling. Raw matching is actually stricter than the normalized path here, which is the safe direction. splitPathSegments collapses // via filter(Boolean), and the protocol-relative guard runs first, so leading // is still caught.

Verification run (local)

• app-rsc-route-matching (27), app-rsc-handler (123), app-page-request (29) — pass
• app-server-action-execution, prerender-route-params, static-image-emission (117 total) — pass
• middleware-runtime + app-router-middleware-next-request + middleware-runtime-trailing-slash (45) — pass
• shims (1186) + entry-templates (41) — pass
• tsc --noEmit on packages/vinext — clean; vp lint — clean on all changed files (the only lint errors are pre-existing TS-version noise in tests/cache-runtime-loading.test.ts, unrelated to this PR)

Non-blocking notes

1. Dead parameter: createNextRequest's normalizedPathname (middleware-runtime.ts:227). Now that mwPathname derives entirely from url.pathname (247-251), the normalizedPathname argument passed at the call site (line 319) is no longer read inside the function. It still lints clean because the call site passes it, but the signature now implies participation it no longer has. Consider dropping the parameter.

2. Stale comment (middleware-runtime.ts:237-246). The comment block still describes the old normalizedPathname-based basePath re-add ("normalizedPathname may already be basePath-stripped ... so for in-basePath requests we re-add"). The logic now keys off url.pathname + !hasBasePath(...). Worth updating so future readers don't assume normalizedPathname drives mwPathname.

3. app-server-action-execution.ts double-decode is now fully eliminated — nice. The old options.matchRoute(options.cleanPathname) calls are replaced with the pre-computed currentRouteMatch/currentRoutePathname threaded from the handler, so action rerenders route on the request-aware pathname. This is a genuine improvement over the earlier "harmless via idempotency" state.

4. Traversal e2e cases still target non-existent routes (/foo/..%252fadmin, /api/health/..%252fadmin). The not.toContain("Protected admin content") assertion is the real check and it holds; pointing one case at an existing sibling would sharpen intent. Minor.

Overall: the raw-match-with-canonicalization approach cleanly separates "decode for matcher gating" from "route on the encoded path," matches Next.js param semantics for both handler and page routes, and closes the double-decode/middleware-bypass class of bug across dev, prod, and Workers. Nice work.

Review: fix(router): avoid repeated App path decoding

Reviewed the current branch state (through 73b9602) end-to-end: the raw-matching path (matchRequestRoute / trieMatchRaw / matchRoutePatternRaw), requestCleanPathname threading, the cleanPathnameIsRequestPathname flag lifecycle, createNextRequest's encoded-pathname switch, the action-execution route-identity threading, dev/prod/worker parity, and the security scenarios. Ran the affected unit suites and tsc/vp lint locally. Verdict: approve — correct, well-scoped, thoroughly tested. Prior review notes #1/#2 (dead param + stale comment) are now fixed by the refactor(middleware): remove stale pathname argument commit.

What I verified independently

1. Raw-matching path (app-rsc-route-matching.ts, route-trie.ts, route-pattern.ts) — matchRequestRoute splits requestCleanPathname with splitPathSegments (no decode), then finalizes with a match-once contract: route handlers get decodeMatchedParams (/api/a%2Fb → a/b), page routes get canonicalizeAppPageParams (/files/a%2561 stays a%2561, %2F stays encoded). Matches Next.js param semantics. New unit cases cover static, dynamic, interception target/source, and route-handler shapes.

2. cleanPathnameIsRequestPathname flag — checked all six cleanPathname mutation sites in app-rsc-handler.ts (648, 691, 727, 752, 972, 1018). Every un-normalizing mutation clears the flag (middleware gated on rewritten, each beforeFiles/afterFiles/fallback rewrite unconditionally). matchCleanPathname() gracefully falls back to matchRoute(cleanPathname) when matchRequestRoute is absent. Coverage is complete.

3. Middleware sees the encoded path — createNextRequest builds mwPathname from url.pathname (raw encoded); the matcher (executeMiddleware) still gates on decoded normalizedPathname. This is exactly Next.js semantics (matcher decides whether middleware runs; nextUrl.pathname reflects the encoded request). The !hasBasePath(requestPathname, basePath) guard correctly re-adds basePath only for Vite-stripped Pages dev URLs.

4. Dot-segment / traversal defense — confirmed the WHATWG URL parser resolves ./.. and %2e-encoded dot segments in url.pathname before requestCleanPathname is derived, so single-encoded traversal collapses identically to the normalized path. Double-encoded %252f stays a literal segment, so ..%252fadmin can't reach a sibling. // collapses via splitPathSegments' filter(Boolean) on both paths, and the protocol-relative guard runs first. Raw matching is stricter (safe direction).

5. Security scenarios — /%2561dmin → matcher decodes to /admin so the /admin matcher fires, but the middleware body reads encoded /%61dmin → no block → matchRequestRoute("/%2561dmin") → 404. /%61dmin → same gating, raw matcher → 404. Both hold against the real /admin page + blocking middleware now in the fixtures. getRawPath is the right call to bypass Playwright's URL normalization on the single-encoded case.

6. Parity — prod (rawUrl restore so normalizeRscRequest decodes exactly once), dev (__vinextOriginalEncodedUrl capture + middlewareUrl threading), workers (built RSC entry). app-server-action-execution.ts double-decode is now eliminated via threaded currentRouteMatch / currentRoutePathname.

Verification run (local)

• app-rsc-route-matching (27), app-rsc-handler (123), app-page-request (29) — pass
• app-server-action-execution, prerender-route-params, static-image-emission (117) — pass
• middleware-runtime + app-router-middleware-next-request (32) — pass
• tsc --noEmit on packages/vinext — clean; vp lint on all changed source — clean

Non-blocking notes

1. Error-boundary / access-fallback intercept lookup uses cleanPathname, not interceptionPathname. entries/app-rsc-entry.ts:906 and :915 call findIntercept(cleanPathname, interceptionContext), whereas the main render path (:828-832, probePage at :884) uses the request-aware interceptionPathname. interceptionPathname is already in scope in this handler (destructured at :744). Practical impact is essentially nil — the intercept route still resolves the same entry; only param values would differ and they aren't read here (only slotKey / sourcePageSegments). Worth aligning for consistency so a future reader doesn't assume these two lookups are intentionally different.

2. Dev middlewareUrl retains a stripped .html suffix (index.ts:4718). middlewareUrl is seeded from req.__vinextOriginalEncodedUrl (captured before Vite normalizes), so it isn't subject to the /index.html and .html stripping applied to url at :4711-4716. The _next/data branch resyncs (middlewareUrl = url at :4839) but the .html case doesn't, so a /about.html dev request would hand middleware a nextUrl.pathname ending in .html. Unlikely for App Router middleware but an asymmetry with url; a resync or a comment explaining the intent would close it. (Flagged in prior reviews; still present.)

3. Traversal e2e cases target non-existent routes (/foo/..%252fadmin, /api/health/..%252fadmin). The not.toContain("Protected admin content") assertion is the real check and it holds; pointing one case at an existing sibling would sharpen intent. Minor.

Overall: the raw-match-with-canonicalization approach cleanly separates "decode for matcher gating" from "route on the encoded path," matches Next.js param semantics for both handler and page routes, and closes the double-decode / middleware-bypass class of bug across dev, prod, and Workers. Nice work.

Review: fix(router): avoid repeated App path decoding

Reviewed the current branch end-to-end at 0daa4b7: the raw-matching path (matchRequestRoute / trieMatchRaw / matchRoutePatternRaw), requestCleanPathname threading and its cleanPathnameIsRequestPathname lifecycle, createNextRequest's encoded-pathname switch, the action-execution route-identity threading, areStaticParamsAllowed canonical matching, dev/prod/worker parity, and the security scenarios. Ran the affected unit suites and tsc locally. Verdict: approve — correct, well-scoped, thoroughly tested. Two non-blocking notes.

What I verified independently

1. matchRoutePattern / trieMatch refactor is behavior-preserving. Both are now thin wrappers: matchRoutePatternRaw/trieMatchRaw capture params without decoding, then the public matchRoutePattern/trieMatch call decodeMatchedParams. I confirmed the extraction is byte-for-byte equivalent to the old inline decodeMatchedParams — important because there are ~14 existing callers (navigation-planner, optimistic-routing, metadata-routes, page-request, router shim, pages-data-url) that must keep decoding. No regression risk there.

2. Raw request matching + match-once contract. matchRequestRoute splits requestCleanPathname with splitPathSegments (no decode), then finalizes with decodeMatchedParams for route handlers (/api/a%2Fb → a/b) vs canonicalizeAppPageParams for page routes (/files/a%2561 stays a%2561, %2F stays %2F). Confirmed round-trip stability of canonicalizeAppPageParam (encodeURIComponent(decodeURIComponent(x))) across UTF-8, %2F, double-encoded, and percent literals. Matches Next.js param semantics for both handler and page routes.

3. areStaticParamsAllowed canonicalization (app-page-request.ts:357) is a required companion change — because page params now arrive canonical-encoded (caf%C3%A9) while generateStaticParams returns decoded (café), the new stringParamMatches compares against both staticValue and encodeURIComponent(staticValue). Correct and necessary; without it, encoded static routes would 404 the prerender allow-list.

4. Dot-segment / traversal defense holds — verified the load-bearing claim. I checked the WHATWG URL parser directly: new URL(...).pathname resolves both literal (/foo/../admin) and %2e/%2e%2e-encoded (/foo/%2e%2e/admin) dot segments to /admin before normalizeRscRequest reads url.pathname. So even though requestCleanPathname skips normalizePath, the only thing normalizePath adds beyond the URL parser is // collapsing — which splitPathSegments' filter(Boolean) handles at match time. Double-encoded %252f stays a literal segment so ..%252fadmin can't reach a sibling. The protocol-relative guard runs on raw url.pathname first. Raw matching is stricter than the normalized path (safe direction).

5. cleanPathnameIsRequestPathname flag lifecycle is complete. All six cleanPathname assignment sites in app-rsc-handler.ts (647, 690, 726, 751, 971, 1017) clear the flag; the middleware site (647) gates the reset on rewritten, and I confirmed in app-middleware.ts:299 that cleanPathname is returned unchanged whenever rewritten is false, so the "cleanPathname changes ⟺ rewritten" invariant holds. matchCleanPathname() also degrades gracefully to matchRoute(cleanPathname) when matchRequestRoute is absent.

6. Middleware sees the encoded path. createNextRequest now builds mwPathname from url.pathname (raw encoded); the matcher still gates on decoded normalizedPathname. The !hasBasePath(requestPathname, basePath) guard correctly avoids double-prefixing for App Router (raw URL carries basePath) while re-adding it for Vite-stripped Pages dev URLs. The removed normalizedPathname param and stale comment (flagged in earlier reviews) are gone in 73b9602.

7. Parity + security. prod (rawUrl restore so normalizeRscRequest decodes exactly once), dev (__vinextOriginalEncodedUrl capture + middlewareUrl threading + basePath resync at 4771-4777), workers (built RSC entry). /%2561dmin → matcher decodes to /admin so the matcher fires, but middleware body reads encoded /%61dmin → no block → raw matcher → 404. /%61dmin → 404. app-server-action-execution.ts double-decode is fully eliminated via the threaded currentRouteMatch/currentRoutePathname.

Verification run (local)

• app-rsc-route-matching (27), app-rsc-handler (123), app-page-request (29) — pass
• app-server-action-execution, prerender-route-params, static-image-emission (117) — pass
• shims + middleware-runtime + app-router-middleware-next-request (1218) — pass
• tsc --noEmit on packages/vinext — clean

Non-blocking notes

1. Dev middlewareUrl retains a stripped .html suffix (index.ts:4721). middlewareUrl is seeded from req.__vinextOriginalEncodedUrl (captured before Vite normalizes), so it isn't subject to the /index.html and .html stripping applied to url at 4711-4716. The _next/data branch resyncs (middlewareUrl = url at 4832) but the .html case doesn't, so a /about.html dev request would hand middleware a nextUrl.pathname ending in .html. Unlikely for App Router middleware but an asymmetry with url — a resync or a comment would close it. (Persistent across prior reviews.)

2. Fragile cross-module invariant at app-rsc-handler.ts:647. The flag reset on 648 is gated on middlewareResult.rewritten, correct only because applyAppMiddleware maintains "cleanPathname changes ⟺ rewritten." A value-based guard (cleanPathnameIsRequestPathname &&= cleanPathname === requestCleanPathname after the assignment) or a one-line comment documenting the dependency would make a future middleware refactor safer. Not a bug today.

3. Traversal e2e cases target non-existent routes (/foo/..%252fadmin, /api/health/..%252fadmin). The not.toContain("Protected admin content") assertion is the real check and it holds; pointing one at an existing sibling would sharpen intent. Minor.

Overall: the raw-match-with-canonicalization approach cleanly separates "decode for matcher gating" from "route on the encoded path," matches Next.js param semantics for both handler and page routes, and closes the double-decode / middleware-bypass class of bug across dev, prod, and Workers. Nice work.

Review: fix(router): avoid repeated App path decoding

I independently re-verified the current branch state (through 7aa4a0b) end-to-end: the raw-matching path (matchRequestRoute / trieMatchRaw / matchRoutePatternRaw), requestCleanPathname threading and its cleanPathnameIsRequestPathname lifecycle, createNextRequest's encoded-pathname switch, the action-execution route-identity threading, areStaticParamsAllowed canonicalization, dev/prod/worker parity, and the security scenarios. Ran the affected unit suites and tsc locally. Verdict: approve — correct, well-scoped, thoroughly tested.

What I verified independently

1. Raw-matching path + match-once contract (app-rsc-route-matching.ts:171-180). matchRequestRoute splits requestCleanPathname with appRscPathnameParts(url, true) (no decode), then finalizes params by route kind: routeHandler routes get decodeMatchedParams (decode once — /api/a%2Fb → a/b), page routes get canonicalizeAppPageParams (decode→re-encode — /files/a%2Fb stays a%2Fb, /files/a%2561 stays a%2561). Matches Next.js param semantics for both handler and page routes. The route-pattern.ts/route-trie.ts refactor into *Raw + decodeMatchedParams wrappers is behavior-preserving for the ~14 existing decode-once callers.

2. cleanPathnameIsRequestPathname flag lifecycle is complete. All six cleanPathname mutation sites in app-rsc-handler.ts (648, 691, 727, 752, 972, 1018) clear the flag — middleware gated on cleanPathname !== normalized.cleanPathname (a value-based guard, which resolves the fragile-rewritten-coupling note from earlier reviews), each beforeFiles/afterFiles/fallback/action rewrite unconditionally. matchCleanPathname() degrades gracefully to matchRoute(cleanPathname) when matchRequestRoute is absent.

3. Middleware sees the encoded path (middleware-runtime.ts). createNextRequest builds mwPathname from url.pathname (raw encoded); the matcher (executeMiddleware) still gates on the decoded normalizedPathname. The basePath && hadBasePath && !hasBasePath(requestPathname, basePath) guard correctly re-adds basePath only for Vite-stripped Pages dev URLs. The dead normalizedPathname param + stale comment (flagged in prior reviews) are removed from createNextRequest; the param correctly remains on runMiddleware/executeMiddleware where it still drives matcher gating.

4. Traversal / dot-segment defense — verified the load-bearing claim directly. requestCleanPathname skips normalizePath, but I confirmed the WHATWG URL parser resolves literal (/foo/../admin), %2e/%2E-encoded (/foo/%2e%2e/admin), and mixed (.%2e) dot segments to /admin before normalizeRscRequest reads url.pathname. Double-encoded %252f / single-encoded %2f stay literal segments (/foo/..%252fadmin → ["foo","..%252fadmin"] → 404), so .. can't reach a sibling. // collapses via splitPathSegments' filter(Boolean); the protocol-relative guard runs on raw url.pathname first. Raw matching is stricter than the normalized path — the safe direction.

5. areStaticParamsAllowed canonicalization (app-page-request.ts) is a required companion change: page params now arrive canonical-encoded while generateStaticParams returns decoded, so the new stringParamMatches compares against both staticValue and encodeURIComponent(staticValue). canonicalizeAppPageParam and this check both key off encodeURIComponent, so they agree. Without it, encoded static routes would 404 the prerender allow-list. prerender-route-params.ts similarly accepts either encoded or decoded values.

6. Security scenarios. /admin → 403 (middleware matcher fires on decoded path and blocks); /%2561dmin → 404 (matcher sees %61dmin ≠ admin, raw matcher finds no route); /%61dmin → 404 (getRawPath bypasses Playwright's URL normalization; raw matcher doesn't decode). Confirmed against the real /admin page + blocking middleware now in the fixtures, and against the new matchRequestRoute unit cases (/%61dmin → null, /%2561dmin → null).

7. Parity + action execution. prod (rawUrl restore so normalizeRscRequest decodes exactly once — comment documents intent), dev (__vinextOriginalEncodedUrl capture + middlewareUrl threading + basePath resync), workers (built RSC entry). app-server-action-execution.ts double-decode is fully eliminated via the threaded currentRouteMatch / currentRoutePathname. The error-boundary/access-fallback intercept lookups in entries/app-rsc-entry.ts (906/915) now use the request-aware interceptionPathname, resolving the consistency gap from a prior review.

Verification run (local)

• app-rsc-route-matching (27), app-page-request (29), app-rsc-handler (123) — pass
• middleware-runtime, app-router-middleware-next-request, app-server-action-execution, prerender-route-params, static-image-emission (149) — pass
• entry-templates (41), routing, route-sorting (201) — pass
• tsc --noEmit on packages/vinext — clean

Non-blocking notes

1. Dev middlewareUrl retains a stripped .html suffix (index.ts:4721). middlewareUrl is seeded from req.__vinextOriginalEncodedUrl (captured before Vite normalizes), so it isn't subject to the /index.html/.html stripping applied to url at 4711-4716. The _next/data branch resyncs (middlewareUrl = url at 4832) and the basePath branch resyncs, but the .html case doesn't — a /about.html dev request would hand middleware a nextUrl.pathname ending in .html. Unlikely for App Router middleware, but an asymmetry with url; a resync (or a comment explaining the intent) would close it. Persistent across prior reviews.

2. Traversal e2e cases target non-existent routes (/foo/..%252fadmin, /api/health/..%252fadmin). The not.toContain("Protected admin content") assertion is the real check and it holds; pointing one at an existing sibling would sharpen the "can't traverse to a real route" intent. Minor.

Overall: the raw-match-with-canonicalization approach cleanly separates "decode for matcher gating" from "route on the encoded path," matches Next.js param semantics for both handler and page routes, and closes the double-decode / middleware-bypass class of bug across dev, prod, and Workers. The value-based flag guard and the eliminated action-execution double-decode are genuine hardening improvements over earlier iterations. Nice work.

Review: fix(router): avoid repeated App path decoding

Reviewed the current branch end-to-end at 2e255c8, focusing on the four commits added since the last review (3c6a84c encoded route parity, 22aaa31 raw config path matching, 37a7447 raw encoded route identity, 2e255c8 encoded dot-segment canonicalization). Ran the affected unit suites and tsc/vp lint locally. Verdict: approve — correct, well-scoped, thoroughly tested across dev, Node prod, and Cloudflare Workers.

What I verified independently

1. New: WHATWG dot-segment canonicalization (request-pipeline.ts + 2e255c8). canonicalizeRequestPathname sets url.pathname = pathname on a throwaway URL to remove literal and %2e-encoded dot segments while preserving unrelated percent escapes. I confirmed byte-for-byte preservation of /%61bout/, /dynamic/a%2561/b%2Fc, /%2f, /%5c, /%252f, and dot-segment removal of /%2e/about→/about, /x/%2E%2e/old-about→/old-about. This is the correct fix for the Node dev/prod-server paths, which route on raw req.url strings and previously did not get the dot-segment resolution that the App Router gets for free via the Request/URL constructor (nodeToWebRequest). Parity is now consistent: App Router (Request-based) and Pages (raw-string) resolve dot segments identically before config/basePath/route matching.

2. Ordering is safe. isOpenRedirectShaped runs before canonicalization in both index.ts (4714-4719) and prod-server.ts (1640), so ///\/%5C/%2F protocol-relative shapes are rejected first. canonicalizeRequestPathname never throws on malformed percent-encoding (leaves bare % untouched), so normalizePathnameForRouteMatchStrict still catches /%E0%A4%A→400 downstream. Verified both directly.

3. matchRoute raw-param extraction (3c6a84c). matchRoute now selects the route on the normalized parts (appRscPathnameParts(url, false)) but extracts param values from the raw parts via extractRawParamsForMatchedRoute, then applies route-kind canonicalization. I checked the load-bearing alignment invariant: normalizePathnameForRouteMatch re-encodes decoded delimiters (%2F stays %2F, a decoded / is re-encoded), so decoding never changes segment count — raw and normalized part arrays stay index-aligned. No misalignment risk.

4. isAppRouteHandlerRoute classification (3c6a84c). Correctly keys off routeHandler != null || typeof __loadRouteHandler === "function" so a lazily-loaded route handler is classified as a handler (decode-once) rather than a page (canonical-encode) before its module loads. __loadRouteHandler is a real generated-manifest field (app-rsc-manifest.ts:323). Good catch — classification is now load-order-stable.

5. Raw config-source matching (22aaa31/37a7447). Config redirects/rewrites/headers now match requestCleanPathname (raw encoded) via the cleanPathnameIsRequestPathname ? requestCleanPathname : cleanPathname pattern, consistently applied at every site. This matches Next.js — /%72ewrite no longer aliases a /rewrite config source. applyRewrite matches on sourcePathname (raw) and substitutes captures from paramsPathname (raw) so Location values retain original encoding. Flag reset at app-rsc-handler.ts:660 now uses didMiddlewareRewrite || cleanPathname !== normalized.cleanPathname (rewrite-authoritative + value-based), which is a hardening over the earlier pure value-based guard.

6. i18n locale transform extended to config headers (37a7447). Verified against Next.js load-custom-routes.ts processRoutes: the :nextInternalLocale source prefix is applied to all route types (redirect/rewrite/header) when i18n is set, while the per-default-locale literal variants are emitted only for redirects (defaultLocales is populated only for type === 'redirect'). vinext's applyLocaleToRoutes mirrors this exactly (defaultLocales = type === "redirect" ? [i18n.defaultLocale] : []). The "destination" in r narrowing correctly handles NextHeader (no destination). The retained-unprefixed-source push (line 1642) is the pre-existing documented divergence, now applied consistently.

Verification run (local)

• request-pipeline (106), app-rsc-route-matching (29), app-rsc-handler (128) — pass
• routing, app-page-request, prerender-route-params, middleware-runtime, pages-request-pipeline, app-server-action-execution, static-image-emission (359) — pass
• shims (1187) + app-rsc-response-finalizer — pass
• tsc --noEmit on packages/vinext — clean; vp lint — clean on changed source (only pre-existing TS7-version noise in tests/cache-runtime-loading.test.ts, unrelated)

Non-blocking notes

1. Dev middlewareUrl retains a stripped .html suffix (index.ts:4736). middlewareUrl is seeded from canonicalOriginalUrl (canonicalized raw), so it isn't subject to the /index.html/.html stripping applied to url at 4726-4731 (routeUrl gets its own stripping; middlewareUrl deliberately keeps .html so middleware sees the real request). The _next/data branch resyncs (middlewareUrl = url at 4864) and the basePath branch resyncs, but the .html case doesn't — a /about.html dev request hands middleware a nextUrl.pathname ending in .html. This appears intentional (middleware should distinguish a real .html request), but the asymmetry with the resyncing branches is worth a one-line comment to prevent a future "fix." Persistent across prior reviews.

2. Traversal e2e cases target non-existent routes (/foo/..%252fadmin, /api/health/..%252fadmin). The not.toContain("Protected admin content") / status 404 assertions are the real check and they hold; the new 2e255c8 dot-segment specs (/docs/x/%2e%2e/rewrite-about → 200, /docs/%2e%2e/about → 404) meaningfully sharpen the "single-encoded dot resolves, double-encoded delimiter stays literal" intent. Minor.

3. canonicalizeRequestPathname also percent-encodes special ASCII (space, <, >, backtick, ", #, ?) via url.pathname, not just dot segments. Harmless in practice (real HTTP request URLs arrive already-encoded on the wire, and App Router's Request constructor applies the identical encoding, so parity holds) — but the docblock claims only dot segments change while "other percent escapes remain untouched." Consider noting that non-percent special chars are also normalized to their canonical encoding, so a future reader doesn't assume the function is a pure dot-segment resolver.

Overall: the raw-match-with-canonicalization approach cleanly separates "decode for matcher gating" from "route on the encoded path," matches Next.js param and config-source semantics for handler and page routes, and the new dot-segment canonicalization closes the last dev/prod parity gap with the Request-based App Router path. Nice work.

Review: fix(router): avoid repeated App path decoding

Focused re-review of the latest commit 0322d8b ("restore encoded path parity after main merge") on top of the already-approved raw-matching implementation. The core raw-match + canonicalization approach and the i18n/config-source changes remain sound (verified against Next.js load-custom-routes.ts). The dev basePath switch to hasBasePath/stripBasePath is a genuine improvement (it fixes a /app vs /application prefix collision and correctly folds in the Vite base). The prod-server 404 reorder is safe — missingBuildAsset is only set for _next/static / app-asset-prefix paths, which never legitimately resolve to a public static-file signal.

One thing in the merge-parity commit stood out that I don't think matches Next.js.

areStaticParamsAllowed case-insensitive comparison diverges from Next.js (non-blocking, but likely wrong)

app-page-request.ts:361-362 changed the static-param comparison from exact to case-insensitive:

// Route matching is case-insensitive, so preserve that behavior for the
// encoded comparison too.
const stringParamMatches = (value: string, staticValue: string): boolean =>
  value.toLowerCase() === encodeURIComponent(staticValue).toLowerCase();

Two problems:

1. The stated justification doesn't hold. vinext route matching is case-sensitive: route-trie.ts:184 does node.staticChildren.get(segment) (a case-sensitive Map lookup), and there is no toLowerCase anywhere in routing/. Next.js is also case-sensitive here — the runtime dynamicParams=false gate is an exact staticPaths?.includes(resolvedUrlPathname) (base-server.ts:2171), and encodedPathname in the prerender manifest is built with plain encodeURIComponent (no lowercasing). So the comment's premise is incorrect.

2. It's unnecessary for the percent-encoding parity it targets, and it over-broadens matching. Both sides already normalize hex to uppercase: encodeURIComponent emits %2F, and the runtime value arrives via canonicalizeAppPageParam = encodeURIComponent(decodeURIComponent(...)), which also emits %2F. So %2f/%2F parity is already handled by the old exact comparison. The only new behavior .toLowerCase() introduces is on literal ASCII case. Simulated:

   generateStaticParams (dynamicParams=false)	request	OLD (exact)	NEW (.toLowerCase)	Next.js
   { id: "AbC" }	/AbC	200	200	200
   { id: "AbC" }	/abc	404	200	404
   { id: "a/b" }	/a%2fb	200	200	200

   The middle row is the regression: with the new code a case-mismatched request renders a dynamicParams=false route that Next.js would 404. It also disagrees with the sibling encoder in prerender-route-params.ts:207-209, which stays exact encodeURIComponent.

No test distinguishes the two (all existing cases use uppercase-canonical %XX), so this passes CI silently. Suggest reverting to the exact comparison (value === encodeURIComponent(staticValue)) and dropping the "route matching is case-insensitive" comment — or, if a real case-insensitive scenario motivated this, add a fixture that exercises it and make the trie matcher case-insensitive too so the two stay consistent.

Verdict

Approve — the encoded-path fix is correct and well-tested end-to-end; the .toLowerCase() divergence above is the only substantive concern and is narrow (literal-ASCII-case dynamic params under dynamicParams=false). Worth fixing in this PR since it's a fresh, untested divergence, but not a blocker for the primary bug class this PR closes.

Review: fix(router): avoid repeated App path decoding

Focused re-review of the six commits added since the last review (0322d8b), which are all about static-params validation for chained + parallel App Router routes (a6f6b71 exact-match, fef7db4 chain parent params, 921f096 preserve ownership, 320ffaa per-result ownership, 7d5e08c preserve after empty parallel generation, 48117ae chain parallel by branch). The core raw-matching / encoded-path fix from earlier reviews is unchanged. Verdict: approve — correct, well-scoped, thoroughly tested. The one substantive concern from my prior review is now resolved.

Prior-review finding is fixed

The areStaticParamsAllowed case-insensitive divergence I flagged on 0322d8b is resolved (app-page-request.ts:389-390): the comparison is now exact value === encodeURIComponent(staticValue), with an accurate comment ("Dynamic route-pattern matching is case-insensitive, but the concrete values returned by generateStaticParams are not"). New fixtures + e2e (static-param-case-parity.spec.ts) explicitly assert /scalar/AbC → 200 while /scalar/abc, /scalar/aBc → 404, and mirror Next.js' app-prefetch-static route shape. This is the exact case-sensitive behavior Next.js has (base-server.ts exact includes), so the parity gap is closed and now guarded by a regression test.

What I verified in the new static-params logic

1. Chained (primary loader-tree) generation (generateChainedStaticParams) walks sources top-down, merging each child into its parent combo and preserving parent combos on empty child results — matching Next's non-PPR generation. The route-group boundary fix (getLayoutGenerateStaticParamsBoundary now skips @slot/(group) segments) correctly attributes a grouped layout's generateStaticParams to the preceding visible dynamic segment. Confirmed by the (default) force-dynamic fixture and the parent-chain e2e (/EU/En → 200; /eu/En, /EU/en, /EU/Fr → 404).

2. Independent (parallel-branch) chains are grouped by independentChain index and each validated against the request with allowMissingValues=true; the pure-chained case is validated exact. I traced the ownership model end-to-end:

   • Empty-object parallel result ([{}, {slug:"parallel"}]) correctly preserves the primary combo and adds the branch's own combo (preserves ownership for each parallel generator result).
   • Parallel generator that omits a key falls back to the primary chain's value via primaryParams seeding, and the missing-key allowMissingValues path only permits keys a slot genuinely doesn't own.
   • Empty parallel result preserves the primary combos rather than dropping them.
   • The legacy raw-array form (normalizeGenerateStaticParams) still produces one identity-keyed chain per source, so AND-across-sources validation is preserved (requires every segment ... to allow the params still 404s on the mismatched leaf). Backward compatible.

3. Production force-dynamic skip (app-page-dispatch.ts:792) — !(isProduction && isForceDynamic) skips the generated-path gate only in prod, matching Next.js (force-dynamic routes are absent from the prerender manifest) while dev keeps exact-matching. Documented with a comment and covered by both a unit test and the keeps force-dynamic routes on the dev exact-match gate e2e.

Verification run (local)

• app-page-request (37), app-segment-config (36), app-page-dispatch (64) — pass
• app-rsc-route-matching, prerender-route-params, routing (167) — pass
• app-rsc-handler (128), middleware-runtime (16), request-pipeline (106), pages-request-pipeline (82) — pass
• vp check on the three changed source files — format/lint/types clean

Non-blocking notes

1. allowMissingValues=true is load-bearing for parallel-slot ownership (app-page-request.ts:398). It's correct here — a slot that doesn't own a param shouldn't constrain it, and the primary chain still supplies that param's value in result.staticParams — but the permissiveness is subtle. A one-line comment at the allowMissingValues branch noting why a missing key is permitted (slot doesn't own the param; the owning chain validates it) would help future readers distinguish it from an accidental bypass.

2. requestParams seeding in generateIndependentStaticParams (app-page-request.ts:463) — {...requestParams, ...parent.params}lets request-controlled values flow intogenerateStaticParamsfor parent params not produced by a preceding source. This is fine because the finalareStaticParamsAllowed` gate is against the generated set, not the request, but it's worth confirming no generator relies on that input being trusted. Not a bug in the validation path.

3. Dev middlewareUrl .html-suffix asymmetry (persistent from prior reviews) and traversal e2e cases targeting non-existent routes — both still stand as minor, non-blocking.

Overall: the static-params rework cleanly separates chained top-down generation from independent parallel-branch chains, matches Next.js ownership + case-sensitivity semantics, and the prior case-insensitive divergence is fixed and regression-tested. Nice work.